upgrade gwc to 3.0.1#643
Conversation
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Just a few questions regarding the jobList notification and when it should connect.
| const ES = window.EventSource; | ||
| const withCredentials = true; | ||
| const timeoutSeconds = 300; | ||
| const retryMsDefault = 8_000; |
There was a problem hiding this comment.
Underscores are ignored in js numbers. I use them to signify orders of magnitude. This is 8 seconds. I originally had it at 10 seconds, so the underscore is particularly nice for readability there
There was a problem hiding this comment.
Thank You for the info. I didn't know and did a quick search but didn't see anything immediately in the first page (mostly because my google terms were using underscore so I got a lot of underscore.js results). Now adjusting the terms I see the info.
| store, | ||
| vuetify, | ||
| provide: { girderRest, notificationBus, vuetify }, | ||
| provide: { girderRest, vuetify }, |
There was a problem hiding this comment.
| provide: { girderRest, vuetify }, | |
| provide: { girderRest, vuetify, notificationBus: girderRest }, |
The Joblist.vue still requires provide of the notificationBus, it looks like your implementation on girderRest meets all the requirements for the JobList.vue component (A very quick look at it and the GWC looks like it implements most of notificationBus minus the polling).
Obviously without this the job list doesn't update properly when the notification for a job is sent and you are looking at the jobs.
| .$promptAttach(); | ||
|
|
||
| /** Start notification stream if everything else succeeds */ | ||
| registerNotifications(girderRest).connect(); |
There was a problem hiding this comment.
Should we rely on the login signal from the restClient to connect instead of doing it here?
If you sit at the login screen it is constantly attempting to reconnect right now because there is a 401 error so it's in the error/reconnect loop. I could be missing something though.
| color="warning" | ||
| class="mx-2" |
There was a problem hiding this comment.
Feel entirely free to ignore: outlined?
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
I'm good with this as is and it addressed all my previous comments. Feel free to modify the datasetID in training to a folder if you want, or you can leave it as an Id. I've noticed we are a bit inconsistent in the jobs panel. It looks like conversions are run on Ids and pipelines are run on the folder name so we do use both in other locations.
| small | ||
| class="mr-1 mb-2" | ||
| > | ||
| <pre>{{ id }}</pre> |
There was a problem hiding this comment.
I like it, but these numbers only mean stuff you and I and people who understand the routing. If it was the folder name that might help a bit more for regular users.
There was a problem hiding this comment.
It wasn't available without adding more props. I agree it's a bit odd. Mostly I wanted a clear visual indicator of quantity.
I could just change it to say "Train on {count} datasets" if that's preferred.
| class="mr-0" | ||
| > | ||
| mdi-menu-right | ||
| mdi-menu-left |
There was a problem hiding this comment.
I'm guessing it's mostly from only knowing right-to-left languages but it feels different. This isn't me requesting a change or anything and I'd wait to see what others say. I played around for a bit seeing if I could do some stuff width nudging and moving it around but nothing was better than what you currently have.
There was a problem hiding this comment.
Yeah, it's odd. If you want to make them open to the right and just not worry about the case where it covers the lower menu, I'm good with that.
2 participants
This removes the notification check every few seconds and should dramatically reduce the amount of noise in server logs.
fixes #629